Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the ability to inject redis clients into the Faye Redis engine. #13

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

suprememoocow
Copy link

This PR allows the Redis client to be injected into the Faye Redis engine, instead of having it instantiated by the engine itself (using the host, port etc options).

Background

We're currently moving across to using Redis Sentinel. In node, we're using Node Redis Sentinel Client to coordinate our Redis connections. This library provides a client which walks like a Redis client and talks like a Redis client, but isn't a standard Redis client (as it maintains a connection to the Sentinel, listening for failover messages, etc).

Implementation

Two extra options have been added to the Faye Redis engine: client and subscriberClient. These are intended to be used together and as an alternative to specifying the Redis connection parameters host, port etc. They should each reference a Redis client (or in our case a Redis-client-like object).

@evantahler
Copy link

👍

This looks like it would work for the myriad of sentinel clients that exist (personally, I picked https://github.com/flyerhzm/redis-sentinel) and would also allow for simpler testing with https://github.com/hdachev/fakeredis as well

@evantahler
Copy link

bump?

@jcoglan
Copy link
Collaborator

jcoglan commented Jun 2, 2014

@evantahler Please don't do that, it's passive-aggressive. I have been having a really rough year and am trying to catch up on my OSS stuff. I'm getting to everything as soon as I can.

@suprememoocow
Copy link
Author

@jcoglan, sorry about the broken build on this PR. I've got no idea why redis seems to be failing on the travis build when running against Node 0.6. All the other builds (node 0.8, etc) pass. Any ideas?

@fatso83
Copy link

fatso83 commented Sep 16, 2015

For the record, 0.6 is no longer part of the Travis build, so it should pass the build checks now.

@puzrin
Copy link

puzrin commented Feb 9, 2021

IMO option createClient would be more flexible.

Also note:

  • multiple connection params are outdated, clients support URL (single string) now.
  • redis pkg dependency is not nice (imo should be dropped, because is weak peer dependency). People may wish ioredis.

@jcoglan what is status of this pakage? Can i help with refresh of outdated codebase? Or i should do the fork?

@fatso83
Copy link

fatso83 commented Feb 10, 2021

@puzrin I am not sure why you even bother asking. This package has not seen a single maintenance commit in seven years. James should have just archived the repo.

@puzrin
Copy link

puzrin commented Feb 10, 2021

@fatso83 it's not difficult to ask an be polite, before switch to gitterhq fork :)

@fatso83
Copy link

fatso83 commented Feb 10, 2021

@puzrin The GitterHQ has not seen any action in years either, so if you encounter any other issues, I would frankly look for an alternative. Still, if it works and scratches your itch, old software is good software :)

@puzrin
Copy link

puzrin commented Feb 10, 2021

@fatso83 fork has ancient look, but seems to solve all real problems. Of cause, i'd prefer to use mainstream and can prepare a set of commits. But need a minimal feedback from @jcoglan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants